fix(iroh): don't hit network for complete blobs&collections #2576
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2576/docs/iroh/ Last updated: 2024-08-02T14:38:15Z |
…nd do not attempt to download from ourselves
Frando
force-pushed
the
fix-download-self
branch
from
22d5acd to
8845025
Compare
|
lgtm, can't approve my own pr 😅 |
dignifiedquire
changed the title
matheus23
reviewed
Aug 2, 2024
matheus23
approved these changes
Aug 2, 2024
Frando
force-pushed
the
fix-download-self
branch
from
527bb93 to
cc9fa77
Compare
|
As discussed in Discord, the previous fix was incorrect as it would report a succeeding download for collections if the root blob is complete even if we had no or only some of the children available locally. I added two commits that fix this:
Also updated the PR description. Please re-review :) |
Frando
force-pushed
the
fix-download-self
branch
from
cc9fa77 to
ccf15d4
Compare
Frando
changed the title
dignifiedquire
commented
Aug 2, 2024
|
|
||
| let mut children: Vec<Hash> = vec![]; | ||
| while let Some(hash) = hash_seq.next().await? { | ||
| children.push(hash); |
There was a problem hiding this comment.
why collect first, instead of directly doing the db checks?
There was a problem hiding this comment.
Because with the PR as-is, we may only emit events here if everything is complete. Otherwise the same events would be emitted again when actually starting the download.
This is suboptimal for sure. See #2586 for a revised approach that does not need to collect events by using a generator and turning get_to_db into a two-step process.
dignifiedquire
commented
Aug 2, 2024
| }, | ||
| ) | ||
| .await? | ||
| .await |
There was a problem hiding this comment.
we should probably check the events in these tests, to make sure they look like we expect them, especially in the collection case
|
closing in favor of #2586 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 5, 2024
…are complete (#2586) ## Description Two changes to the downloader: * Never try to download from ourselves. If the only provider node added is our own node, fail with error "no providers". * The actual download request flow is turned into a generator (while keeping API compatibility for the existing `get_to_db` public function). A new `get_to_db_in_steps` function either runs to completion if the requested data is fully available locally, or yields a `NeedsConn` struct at the point where it needs a network connection to proceed. The `NeedsConn` has an `async proceed(self, conn: Connection)`, which must be called with a connection for the actual download to start. This two-step process allows the downloader to check if we should dial nodes at all, or are already done without doing anything, while emitting the exact same flow of events (because we run the same loop) to the client. To achieve this, `get_to_db` now uses a genawaiter generator internally. This means that the big loop that is the iroh-blobs protocol request flow does not have to be changed at all, only that instead of a closure we yield and resume, which makes this much easier to integrate into an external state machine like the downloader. The changes needed for this for the downloader are a bit verbose because the downloader itself is generic over a `Getter`, with impls for the actual impl and a test impl that does not use networking; therefore the new `NeedsConn` state has to be modeled with an additional associated type and trait here. This PR adds three tests: * Downloading a missing blob from the local node fails without trying to connect to ourselves * Downloading an existing blob succeeds without trying to download * Downloading an existing collection succeeds without trying to download Closes #2575 Replaced #2576 ## Notes and open questions ## Breaking changes None, only an API addition to the public API of iroh_blobs: `iroh_blobs::get::check_local_with_progress_if_complete` --------- Co-authored-by: dignifiedquire <me@dignifiedquire.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Two changes to the downloader:
The second point turned out to be a bit more complicated:
FoundLocalevents so that the state tracking at the client works the same no matter if an actual download was performed or not.Both these things are now implemented, through a new function
check_local_with_progress_if_completein iroh_blobs that is called before starting the download. It checks if the blob is fully available locally (and all children too for hashseqs) and if so returnstrue, and emits theFoundLocalevents. No events are emitted if the blob is not completely available, as in that case we start the actual download machinery, which will emit theFoundLocalevents too on the go.Adds three tests:
Closes #2575
Notes and open questions
The API around the new function and the exisitng
get_to_dbcould be improved, and we could skip the double-checking, but that would be a more involved change to a public API that I didn't want to make.Breaking changes
None, only an API addition to the public API of iroh_blobs:
iroh_blobs::get::check_local_with_progress_if_complete